Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: move node_modules out of edx-platform repo #18

Closed
wants to merge 11 commits into from

Conversation

kdmccormick
Copy link
Owner

@kdmccormick kdmccormick commented Dec 21, 2022

Description

Background: NPM packages were installed into the openedx image at
/openedx/edx-platform/node_modules. So, when one mounted their own copy of
edx-platform, the built-in node_modules folder was overriden. This
forced edx-platform developers to re-run npm install, which is
redundant, slow, resource-intensive, and added complexity to the first-time
developer setup process. If one forgot to run npm install, their
LMS/CMS frontend would be broken. And, when edx-platform's NPM requirements
were updated, developers would not receive these updates from the openedx
image; they would need to think to re-run npm install themselves.

The solution: Move /openedx/edx-platform/node_modules to
/openedx/node_modules. Note that this new location is outside of the
edx-platform repository. So, when a developer mounts their own copy of
edx-platform, the image's node_modules will remain intact.

Supporting information

Blocked by:

Closes:

Testing instructions

TBD

Deadline

TBD

Other information

TBD

@kdmccormick kdmccormick force-pushed the kdmccormick/global-node-modules branch 3 times, most recently from 2502fe0 to 7e769c5 Compare December 22, 2022 19:40
@kdmccormick kdmccormick force-pushed the kdmccormick/global-node-modules branch 5 times, most recently from 6ed1c5d to 64e0d8b Compare January 10, 2023 21:21
@kdmccormick kdmccormick changed the title perf: globally install node_modules perf: move node_modules out of edx-platform repo Jan 10, 2023
@kdmccormick kdmccormick changed the base branch from master to nightly January 10, 2023 21:36
@kdmccormick kdmccormick force-pushed the kdmccormick/global-node-modules branch 2 times, most recently from e8fc7e5 to a5d27a1 Compare January 10, 2023 22:18
regisb and others added 2 commits January 17, 2023 12:12
In scriv 1.1.0 the GitHub release description can be templated:
nedbat/scriv#61
https://github.com/nedbat/scriv/releases/tag/1.1.0

This means that we can finally get rid of our ugly scripts to generate the
release description \o/
Carlos Muniz and others added 6 commits January 18, 2023 07:37
Adds `from __future__ import annotations` to the top of every module,
right below the module's docstring. Replaces any usages of t.List,
t.Dict, t.Set, t.Tuple, and t.Type with their built-in equivalents:
list, dict, set, tuple, and type. Ensures that make test still passes
under Python 3.7, 3.8 and 3.9.
When a user registers, they receive a confirmation email. This email contained
two links to "https://example.com/..." urls. This was caused by the fact that
the default site, indicated by SITE_ID=1, was example.com. We resolve this
issue by setting instead SITE_ID=2, which should point to the site with the LMS
domain name.

This is a potentially breaking change for platforms that have manually set to 1
the id of the LMS site in the database. These platforms should now set
SITE_ID=1 via a plugin.

Alternatives we have considered include modifying the id field of the LMS site
in the database. Unfortunately such a change would have important consequences,
as the site ID is used as a foreign key for other models.

Note that non-https sites still include https links in the registration emails.
This is because the "https" scheme is hardcoded by the "ensure_url_is_absolute"
utility function. So there is nothing we can do about this without making
changes upstream.

Close overhangio#572.
The LMS and CMS were producing lots of logs similar to:

	cms_1                        | 2023-01-17 15:30:11,359 INFO 7 [openedx.core.djangoapps.cors_csrf.helpers] [user 7] [ip 31.223.46.44] helpers.py:64 - Origin 'https://studio.demo.openedx.overhang.io' was not in `CORS_ORIGIN_WHITELIST`; full referer was 'https://studio.demo.openedx.overhang.io/learning/course/course-v1:edX+DemoX+Demo_Course/home' and requested host was 'studio.demo.openedx.overhang.io'; CORS_ORIGIN_ALLOW_ALL=False

These warnings are produced by openedx.core.djangoapps.cors_csrf.helpers. I
don't think they indicate any problem, but they pollute the logs. They are
resolved by adding the "http(s)://<lms/cms host>" to CORS_ORIGIN_WHITELIST in
production, so we did just that.
Adds `from __future__ import annotations` to the top of every module,
right below the module's docstring. Replaces any usages of t.List,
t.Dict, t.Set, t.Tuple, and t.Type with their built-in equivalents:
list, dict, set, tuple, and type. Ensures that make test still passes
under Python 3.7, 3.8 and 3.9.
@kdmccormick kdmccormick force-pushed the kdmccormick/global-node-modules branch from a5d27a1 to 45e9e40 Compare January 18, 2023 19:22
regisb and others added 3 commits January 19, 2023 20:35
Background: NPM packages were installed into the openedx image at
/openedx/edx-platform/node_modules. So, when one mounted their own copy of
edx-platform, the built-in node_modules folder was overriden. This
forced edx-platform developers to re-run ``npm install``, which is
redundant, slow, resource-intensive, and added complexity to the first-time
developer setup process. If one forgot to run ``npm install``, their
LMS/CMS frontend would be broken. And, when edx-platform's NPM requirements
were updated, developers would not receive these updates from the openedx
image; they would need to think to re-run ``npm install`` themselves.

The solution: Move /openedx/edx-platform/node_modules to
/openedx/node_modules. Note that this new location is outside of the
edx-platform repository. So, when a developer mounts their own copy of
edx-platform, the image's node_modules will remain intact.

Closes openedx-unsupported/wg-developer-experience#150
@kdmccormick kdmccormick force-pushed the kdmccormick/global-node-modules branch from 45e9e40 to 9cff095 Compare January 20, 2023 17:40
@kdmccormick kdmccormick deleted the kdmccormick/global-node-modules branch August 18, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants